feat: add admin FORCE_RELOAD command - #1502
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
018c594 to
742c432
Compare
742c432 to
8838e5d
Compare
| // TODO: Say that this fails for some reason; it already internally re-tries multiple times. | ||
| // should we Error because not all transactions are terminated? Ignore it? |
There was a problem hiding this comment.
I wanted to get some input on this before implementing a solution for handling errors. I think that if we're guaranteeing all transactions are terminated, then we probably want to exit early with an error to the client and not reload.
However, what if there were some circumstance (e.g. no free connection slots on the server), which prevents us from terminating forever? To handle cases like that, should we have two different variants of this command (or perhaps a parameter) to "override" a potential failure in terminating a transaction?
|
I'm curious what you think about an alternative approach. Instead of making Postgres cancel queries, do it from our side. You can add a pgdog/pgdog/src/frontend/client/query_engine/query.rs Lines 69 to 73 in 7594279 The other branch can be a For clients that are idle-in-transaction, you could do the same here: pgdog/pgdog/src/frontend/client/mod.rs Line 658 in 7594279 Guard will clean up the abandoned transaction.
Do all of this after setting the pool to offline, so no new queries are accepted. I'm thinking this should be faster than opening a new conn to Postgres for each connection pool, and also guarantees that this command is processed by us. |
|
I definitely agree; I wasn't a fan of it either. Thanks! I pushed a commit with some revisions representing a draft of your suggestions. Also, I added another test for cancelling during a query using
I added internal support for per-pool cancellation, but it definitely adds some extra complexity (the Box::pin, futures::select_all, a Binding method, ..). Is this something we want to support in the command now, or is that structure we want for potentially doing something in the future? |
| } | ||
|
|
||
| /// Fetch all `CancellationToken`s for the backend. | ||
| pub(crate) fn cancellation_tokens(&mut self) -> Vec<CancellationToken> { |
There was a problem hiding this comment.
I think you should make these Cluster-scoped. Each client will be connected to at most one cluster, so you won't need a Vec anymore (1 less allocation in the hot path).
Also, you're cancelling all pools every time, so having it live on the pools isn't necessary, just the cluster will do.
…aring a Cluster token
2 participants
Adds an admin
FORCE_RELOADcommand, which acts as a normalRELOADplus guarantees all in-flight transactions are terminated prior to the command's conclusion (and subsequent transactions are sent to the newPools).TODO:
PR for documentation of this command: pgdogdev/docs#111
Fixes #1472.